mi: Additional define options for older systems with SIOCMCTPALLOC#1019
mi: Additional define options for older systems with SIOCMCTPALLOC#1019igaw merged 1 commit intolinux-nvme:masterfrom
Conversation
|
@jk-ozlabs , I'd love your feedback here. I stumbled across an issue in my build environment where I do have the Mctp.h header, but it doesn't include these defines or structures. However, my target environment will support these, so I need to build for that. I also didn't want to force these IOCTLs to be activated if a system didn't support it, so I've added a USE_SIOCMCTPALLOC define to activate it in this case. |
|
Makes sense - there are some considerations about which definitions may be available at which points in the kernel releases, so I'll need to have a more thorough look through those. I'll get a review done shortly. |
|
@jk-ozlabs any new thoughts on this? |
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
Apologies for the delay - managed to get the kernel archaeology done for the timing of the uapi changes. A couple of comments inline.
|
|
||
| #endif /* !AF_MCTP */ | ||
|
|
||
| #if !defined(MCTP_TAG_PREALLOC) && defined(USE_SIOCMCTPALLOC) |
There was a problem hiding this comment.
MCTP_TAG_PREALLOC hit the uapi at the same time as the tag control ioctls (SIOCMCTPALLOCTAG and SIOCMCTPDROPTAG), so keying off that makes sense.
however, USE_SIOMCTPALLOC doesn't appear anywhere else, so I'm not sure what your intention there is?
There was a problem hiding this comment.
My intention was we don't want ALL builds to include this on a system that doesn't have these headers. Because maybe the dev is going to run it on the same system without this support. So I added this extra new define for them to use to enable this feature.
There was a problem hiding this comment.
Because maybe the dev is going to run it on the same system without this support
but we should already handle that case at runtime:
Lines 194 to 202 in 80763da
.. or are you after different behaviour here?
There was a problem hiding this comment.
Oh!. I didn't realize this was how it works. Knowing this, I'll remove my extra define as it doesn't really make much sense.
chorkin
force-pushed
the
user/chorkin/defines
branch
from
4e69b48 to
80763da
Compare
|
Just to clarify this: the The use-case for this would be: "set I'm definitely up for introducing the backwards-compat definitions, to allow the build environment to be unified. However, if we're taking this approach, we should then always attempt the ioctls (ie, remove the later If the current failure code isn't appropriate for your usage, let me know. However, I think a one-time warning is justified, as that means the MPR responses will not work. |
chorkin
force-pushed
the
user/chorkin/defines
branch
from
80763da to
3856fb7
Compare
|
OK, looks better, but now that |
Older systems (WSL for example) may not include SIOCMCTPALLOC defines and structs and when enabling in MCTP code will cause build errors. This is a workaround. Signed-off-by: Chuck Horkin <[email protected]>
chorkin
force-pushed
the
user/chorkin/defines
branch
from
3856fb7 to
b750907
Compare
|
@igaw can we merge this? |
|
Sorry for the delay, as usual trying to do too many things at the same time. Thanks! |
3 participants
Older systems (WSL for example) may not include SIOCMCTPALLOC defines and structs and when enabling in MCTP code will cause build errors. This is a workaround.